fix(ci,api-server): add control-plane + mcp to build pipelines, fix migration parameter mismatch#1426
Merged
mergify[bot] merged 10 commits intomainfrom Apr 22, 2026
Merged
Conversation
…hancements, and Kustomize overlays from alpha PR 6 of alpha-to-main migration (combined with PR 7 — manifests). Runners: - gRPC transport for session message streaming - gRPC client for control-plane token endpoint - Inbox and session messages APIs with delta buffer - Credential system: fetch/populate/clear, gh CLI wrapper - SSE flush-per-chunk, unbounded tap queue - CP OIDC token for backend credential fetches (RSA keypair auth) - New deps: cryptography, grpcio, protobuf - Tests: grpc_client, grpc_transport, grpc_writer, events_endpoint, app_initial_prompt, expanded bridge_claude and shared_session_credentials Manifests: - mpp-openshift overlay: NetworkPolicy, gRPC Route, CP token Service, RBAC, MCP sidecar, RoleBinding namespace fixes - production overlay updates - openshift-dev overlay - Removed deprecated cluster-reader overlay - All overlays pass kustomize build Migration plan updated: PRs 1-5 marked merged, PR 6+7 combined. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
TestGhWrapper._cleanup() used stale top-level imports of _GH_WRAPPER_PATH
(always ""), causing Path(".").unlink() → IsADirectoryError. Now reads from
the module object with empty-string guards. Also fixes e2e workflow Docker
build context from components/runners to components/runners/ambient-runner
so pyproject.toml is found during image build.
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
clear_runtime_credentials was deleting the Google Workspace credentials file between turns, causing workspace-mcp to fall back to an inaccessible localhost OAuth flow. The file is now intentionally preserved. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Tests were refactored to remove CREDENTIAL_IDS but _fetch_credential
still requires it to look up the credential_id and build the API URL.
Without it, the function returns {} immediately without hitting the
test HTTP server. Also restores credential_id-based response matching
in the lifecycle test.
713 passed, 11 skipped locally.
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
…hot tests The session-list screenshot test failed because setTheme clicked the toggle button before it was visible in the DOM. Add explicit visibility wait with 10s timeout to setTheme, matching the pattern already used in the waitForThemeToggle setup step. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…requests
The websocket package's agui_proxy.go used plain http.Transport / ad-hoc
http.Client instances that bypassed the runnerTransport session-token layer.
Every runner call from the websocket handlers (connectToRunner, interrupt,
feedback, capabilities, mcp-status, task stop/output/list, between-run
listener) lacked the X-Ambient-Session-Token header, causing HTTP 401s
from runners with AGUI_TOKEN middleware enabled.
- Export NewRunnerTransport from handlers package
- Wrap runnerHTTPClient with handlers.NewRunnerTransport
- Add runnerShortClient (10s) and runnerMediumClient (30s) with token transport
- Replace all 7 ad-hoc http.Client{} calls with the named clients
- Add TDD tests: transport type assertion, end-to-end token injection,
missing-secret graceful degradation
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
…on parameter mismatch Two unrelated production issues: 1. vteam_control_plane image was never built by CI — missing from both components-build-deploy.yml and prod-release-deploy.yaml. The deployment references quay.io/ambient_code/vteam_control_plane:latest which doesn't exist, causing ImagePullBackOff. Adds ambient-control-plane to the build matrix, path triggers, kustomize image pinning, and the release DEPLOY_MAP. 2. ambient-api-server migration init container crashes with "pq: got 2 parameters but the statement requires 1". GORM's FirstOrCreate extracts non-zero primary key fields as additional WHERE conditions, but the SQL placeholder count doesn't match when using .Table(). Fix: use Attrs() to defer ID assignment to create-time only, keeping the WHERE clause clean. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a new ambient-mcp component (source, build, Dockerfile, server, client, token exchange, mention resolution, MCP tools, and tests), expands CI/CD workflows to include ambient-control-plane and ambient-mcp, refactors GORM role seeding to use GORM Attrs(), and centralizes runner HTTP transport/clients plus related tests. Changes
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Contributor
|
| File | Component | Mode |
|---|---|---|
components/runners/ambient-runner/.mcp.json |
runner | warn |
components/runners/ambient-runner/architecture.md |
runner | warn |
components/runners/ambient-runner/pyproject.toml |
runner | warn |
No action required — these components are in warn mode. Consider using the component's agent workflow for future changes.
📖 Specs: Runner Spec · Runner Constitution
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
components/ambient-api-server/plugins/roles/migration.go (1)
109-119: Add a regression test for this migration pattern.Given this caused a production init crash, add one migration-focused test that asserts
FirstOrCreatewith this pattern succeeds when the role already exists and when it does not. It will guard both this file and the mirrored credentials migration from ORM behavior regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ambient-api-server/plugins/roles/migration.go` around lines 109 - 119, Add a migration-focused regression test that covers the FirstOrCreate pattern used in the roles migration: specifically exercise tx.Table("roles").Where("name = ?", r.name).Attrs(...).FirstOrCreate(&row) (the roleRow construction that uses api.NewID(), Name/DisplayName/Description/Permissions/BuiltIn) in two scenarios—one where the role already exists and one where it does not—asserting the call completes without error and preserves existing values when present; mirror the same test pattern for the credentials migration to guard against ORM behavior regressions (use the same tx.Table/Where/Attrs/FirstOrCreate flow and verify expected row values and no crashes).components/backend/websocket/agui_proxy_test.go (1)
125-130: Minor: URL parsing assumeshttp://prefix.
rewriteHostTransportslicesrealURL[len("http://"):]directly. This works becausehttptest.NewServerreturns HTTP URLs, but consider usingurl.Parsefor robustness if this helper gets reused:u, _ := url.Parse(t.realURL) rewritten.URL.Host = u.HostNot blocking—current usage is safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy_test.go` around lines 125 - 130, In rewriteHostTransport.RoundTrip the code assumes t.realURL starts with "http://" and slices off a fixed prefix; instead parse t.realURL with url.Parse and assign rewritten.URL.Host = parsed.Host (and optionally parsed.Scheme to rewritten.URL.Scheme) so the host is extracted robustly; update the RoundTrip implementation to use url.Parse on the realURL field before modifying rewritten.URL..github/workflows/prod-release-deploy.yaml (1)
239-240: Centralize component metadata to prevent workflow drift.The component catalog is duplicated across
.github/workflows/prod-release-deploy.yamland.github/workflows/components-build-deploy.yml. This exact drift caused the prior production miss; consider sourcing both matrices from one canonical file or reusable workflow output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/prod-release-deploy.yaml around lines 239 - 240, The duplicated component catalog (entries like "ambient-api-server" and "ambient-control-plane" in the prod-release-deploy and components-build-deploy workflows) must be centralized: extract the canonical component list into one source (a single YAML/JSON file or a reusable workflow that outputs matrix.components) and update both workflows to load matrix.components from that source (replace the inline arrays in prod-release-deploy and components-build-deploy with a step that reads the canonical file or consumes the reusable workflow output). Ensure the workflows still populate the matrix key used by the jobs (matrix.components) and remove the duplicated hard-coded component objects so future changes are made in only the canonical source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/prod-release-deploy.yaml:
- Around line 239-240: The duplicated component catalog (entries like
"ambient-api-server" and "ambient-control-plane" in the prod-release-deploy and
components-build-deploy workflows) must be centralized: extract the canonical
component list into one source (a single YAML/JSON file or a reusable workflow
that outputs matrix.components) and update both workflows to load
matrix.components from that source (replace the inline arrays in
prod-release-deploy and components-build-deploy with a step that reads the
canonical file or consumes the reusable workflow output). Ensure the workflows
still populate the matrix key used by the jobs (matrix.components) and remove
the duplicated hard-coded component objects so future changes are made in only
the canonical source.
In `@components/ambient-api-server/plugins/roles/migration.go`:
- Around line 109-119: Add a migration-focused regression test that covers the
FirstOrCreate pattern used in the roles migration: specifically exercise
tx.Table("roles").Where("name = ?", r.name).Attrs(...).FirstOrCreate(&row) (the
roleRow construction that uses api.NewID(),
Name/DisplayName/Description/Permissions/BuiltIn) in two scenarios—one where the
role already exists and one where it does not—asserting the call completes
without error and preserves existing values when present; mirror the same test
pattern for the credentials migration to guard against ORM behavior regressions
(use the same tx.Table/Where/Attrs/FirstOrCreate flow and verify expected row
values and no crashes).
In `@components/backend/websocket/agui_proxy_test.go`:
- Around line 125-130: In rewriteHostTransport.RoundTrip the code assumes
t.realURL starts with "http://" and slices off a fixed prefix; instead parse
t.realURL with url.Parse and assign rewritten.URL.Host = parsed.Host (and
optionally parsed.Scheme to rewritten.URL.Scheme) so the host is extracted
robustly; update the RoundTrip implementation to use url.Parse on the realURL
field before modifying rewritten.URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 58d72c07-2da1-4b12-9973-da1dd62eb69f
📒 Files selected for processing (7)
.github/workflows/components-build-deploy.yml.github/workflows/prod-release-deploy.yamlcomponents/ambient-api-server/plugins/credentials/migration.gocomponents/ambient-api-server/plugins/roles/migration.gocomponents/backend/handlers/sessions.gocomponents/backend/websocket/agui_proxy.gocomponents/backend/websocket/agui_proxy_test.go
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| req.Header.Set("Content-Type", "application/json") | ||
|
|
||
| resp, err := (&http.Client{Timeout: 10 * time.Second}).Do(req) | ||
| resp, err := runnerShortClient.Do(req) |
| req.Header.Set("Content-Type", "application/json") | ||
|
|
||
| resp, err := (&http.Client{Timeout: 10 * time.Second}).Do(req) | ||
| resp, err := runnerShortClient.Do(req) |
| return | ||
| } | ||
| resp, err := (&http.Client{Timeout: 10 * time.Second}).Do(req) | ||
| resp, err := runnerShortClient.Do(req) |
| return | ||
| } | ||
| resp, err := (&http.Client{Timeout: 10 * time.Second}).Do(req) | ||
| resp, err := runnerShortClient.Do(req) |
| req.Header.Set("Content-Type", "application/json") | ||
|
|
||
| resp, err := (&http.Client{Timeout: 10 * time.Second}).Do(req) | ||
| resp, err := runnerShortClient.Do(req) |
| } | ||
|
|
||
| resp, err := (&http.Client{Timeout: 30 * time.Second}).Do(req) | ||
| resp, err := runnerMediumClient.Do(req) |
| } | ||
|
|
||
| resp, err := (&http.Client{Timeout: 10 * time.Second}).Do(req) | ||
| resp, err := runnerShortClient.Do(req) |
The ambient-mcp component (Go MCP server providing 16 tools for sessions, agents, projects, and watch) was never migrated from the alpha branch. Manifests and control-plane code on main already reference quay.io/ambient_code/vteam_mcp but no source or CI pipeline existed to build it. - Copy components/ambient-mcp/ from upstream/alpha (16 files) - Add ambient-mcp to components-build-deploy.yml (path triggers, build matrix, kustomize image pinning in both deploy jobs) - Add ambient-mcp to prod-release-deploy.yaml (build matrix, comp_image loop for kustomize pinning) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
markturansky
changed the title
Contributor
Merge Queue Status
This pull request spent 12 seconds in the queue, including 2 seconds running CI. Required conditions to merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CI: ambient-control-plane — Added
ambient-control-planeto bothcomponents-build-deploy.ymlandprod-release-deploy.yaml(build matrix, path triggers, kustomize image pinning in deploy jobs). This was completely missing from CI, causingvteam_control_planeto have nolatestor release tags.CI: ambient-mcp — Migrated
components/ambient-mcp/source fromalphabranch (16 files: Go MCP server with 16 tools, Dockerfile, tests). Added to both CI workflows (build matrix, path triggers, kustomize image pinning). Manifests and control-plane code on main already referencequay.io/ambient_code/vteam_mcpbut no source or CI pipeline existed.api-server migration fix — Fixed GORM
FirstOrCreateparameter mismatch inplugins/roles/migration.goandplugins/credentials/migration.go. GORM extracted pre-populatedIDas an additional WHERE bind parameter, but the SQL only had 1 placeholder from.Where("name = ?", ...). Changed to use.Attrs()which defers field assignment to create-time only.Test plan
go vet ./...passes forambient-mcpandambient-api-servergo build ./...passes forambient-api-server:alphatag as immediate fix for control-plane ImagePullBackOff🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests